Skip to content

Feature | Two-Factor Audit Service#134

Open
matiasperrone-exo wants to merge 1 commit into
feat/mfa-device-trust-servicefrom
feat/two-factor-audit-service---cu-86ba2z5gz
Open

Feature | Two-Factor Audit Service#134
matiasperrone-exo wants to merge 1 commit into
feat/mfa-device-trust-servicefrom
feat/two-factor-audit-service---cu-86ba2z5gz

Conversation

@matiasperrone-exo
Copy link
Copy Markdown
Contributor

@matiasperrone-exo matiasperrone-exo commented May 26, 2026

ref.: https://app.clickup.com/t/86ba2z5gz

Changes:

New Files

File Purpose
app/Services/Auth/ITwoFactorAuditService.php Interface declaring the log() contract
app/Services/Auth/TwoFactorAuditService.php Concrete implementation
tests/Unit/TwoFactorAuditServiceTest.php Unit test suite (6 cases)

Modified Files

File Change
app/Services/Auth/TwoFactorServiceProvider.php Registers ITwoFactorAuditService → TwoFactorAuditService as a singleton
phpunit.xml Adds the Two Factor Authentication Test Suite

Design Decisions

  • Single method interfaceITwoFactorAuditService::log(User, eventType, method, ipAddress, ?metadata) keeps the contract minimal and explicit. Validation of allowed event types
    and methods is delegated to TwoFactorAuditLog entity setters (throws InvalidArgumentException on unknown values).

  • Sync persistence — the audit log is flushed immediately ($repository->add($log, true)) to guarantee the record lands before the request finishes, regardless of OTEL availability.

  • OTEL is optionalEmitAuditLogJob is only dispatched when opentelemetry.enabled is true, keeping the happy path free of queue overhead in environments where telemetry is
    disabled.

  • two_factor.success derivation — only EventChallengeFailed maps to false; every other event type (issued, succeeded, device-trusted, etc.) is treated as a
    successful/informational outcome.


OTLP Attributes Emitted

When OpenTelemetry is enabled the following structured attributes are sent under
the two_factor.audit message:

Attribute Type Description
two_factor.event_type string One of the TwoFactorAuditLog::Event* constants
two_factor.method string One of the TwoFactorAuditLog::Method* constants
two_factor.user_id int The authenticated user ID
two_factor.ip_address string Client IPv4 / IPv6 address
two_factor.success bool false only for challenge_failed
two_factor.device_trusted bool true only for device_trusted events
elasticsearch.index string Target index (logs-audit by default)

Tests

tests/Unit/TwoFactorAuditServiceTest.php

Test What it covers
testLogPersistsTwoFactorAuditLogWithCorrectFields All entity fields are set before repository flush
testLogEmitsOtlpAttributes Correct OTLP payload is dispatched when OTEL is enabled
testLogEmitsSuccessFalseForChallengeFailed two_factor.success = false for failed challenges
testLogAcceptsNullMetadata Nullable metadata does not break persistence
testInvalidEventTypeThrowsInvalidArgumentException Guard against unknown event types
testInvalidMethodThrowsInvalidArgumentException Guard against unknown 2FA methods

Requested GOAL

Current state

No 2FA-specific audit service exists. MFA events would be inconsistently logged or only visible through generic application logs.

Target state

TwoFactorAuditService provides a centralized audit primitive that persists TwoFactorAuditLog records and emits OTLP audit attributes through the existing fntech-audit pipeline.

This ticket provides audit infrastructure only. It does not wire audit calls into controller methods, strategies, or device-trust flows. That wiring belongs to Ticket 9/10, where the real MFA flow exists.

TASKS

  • Create ITwoFactorAuditService interface with:
    • log(User $user, string $eventType, string $method, string $ipAddress, ?array $metadata = null): void
  • Implement TwoFactorAuditService.
  • Validate eventType against:
    • challenge_issued
    • challenge_succeeded
    • challenge_failed
    • enrollment_changed
    • device_trusted
    • device_revoked
    • recovery_used
    • settings_changed
  • Validate method against:
    • email_otp
    • sms_otp
    • totp
    • passkey
    • recovery
  • Create and persist TwoFactorAuditLog entity.
  • Store metadata as JSON/null.
  • Capture user_agent from request context when available, or accept a future method signature change if project conventions require explicit user_agent.
  • Emit OTLP attributes following existing fntech-audit pipeline conventions:
    • two_factor.event_type
    • two_factor.method
    • two_factor.user_id
    • two_factor.ip_address
    • two_factor.success
    • two_factor.device_trusted
  • Register ITwoFactorAuditService in TwoFactorServiceProvider.
  • Write unit test: log() persists TwoFactorAuditLog with correct fields.
  • Write unit test: log() emits OTLP attributes.
  • Write unit test: log() accepts null metadata.
  • Write unit test: invalid event type throws InvalidArgumentException.
  • Write unit test: invalid method throws InvalidArgumentException.

ACCEPTANCE CRITERIA

  • log() persists TwoFactorAuditLog with all required fields.
  • Valid event types are accepted.
  • Invalid event types are rejected.
  • Valid methods are accepted.
  • Invalid methods are rejected.
  • Metadata null is stored as null.
  • Metadata array is JSON-serializable and stored correctly.
  • OTLP attributes match project audit conventions.
  • Service is injectable via ITwoFactorAuditService.
  • All unit tests pass.

DEVELOPMENT NOTES

Key files:

  • New: app/Services/Auth/ITwoFactorAuditService.php
  • New: app/Services/Auth/TwoFactorAuditService.php
  • New: tests/Unit/TwoFactorAuditServiceTest.php
  • Modified: app/Providers/TwoFactorServiceProvider.php

Gotchas:

  • This service is a primitive. It should not know about UserController::verify2FA(), verify2FARecovery(), resend2FA(), or concrete MFA flow ordering.
  • Do not add controller/strategy integration tests here. Integration belongs in Ticket 9/10.
  • Follow existing audit pipeline patterns rather than inventing a new OTLP abstraction.

Risks:

  • Risk: audit service becomes a dumping ground for MFA orchestration. Mitigation: keep API limited to log().

Out of scope:

Controller wiring, strategy wiring, device-trust wiring, Elastic dashboards, audit UI, retention policy.

Summary by CodeRabbit

Release Notes

  • New Features

    • Added audit logging for two-factor authentication events, including tracking of authentication methods, client IP addresses, and optional metadata.
    • Integrated with OpenTelemetry for enhanced observability when enabled.
  • Tests

    • Added comprehensive unit tests covering audit log persistence, telemetry emission, and validation of event types and methods.

Review Change Stack

@coderabbitai
Copy link
Copy Markdown

coderabbitai Bot commented May 26, 2026

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 66.67% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The pull request title 'Feature | Two-Factor Audit Service' accurately summarizes the main change—the introduction of a new two-factor audit service with interface, implementation, service provider binding, and comprehensive tests.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch feat/two-factor-audit-service---cu-86ba2z5gz

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@matiasperrone-exo matiasperrone-exo self-assigned this May 26, 2026
@github-actions
Copy link
Copy Markdown

📘 OpenAPI / Swagger preview

➡️ https://OpenStackweb.github.io/openstackid/openapi/pr-134/

This page is automatically updated on each push to this PR.

@matiasperrone-exo matiasperrone-exo force-pushed the feat/two-factor-audit-service---cu-86ba2z5gz branch from fad9611 to ae4e42f Compare May 26, 2026 18:52
@github-actions
Copy link
Copy Markdown

📘 OpenAPI / Swagger preview

➡️ https://OpenStackweb.github.io/openstackid/openapi/pr-134/

This page is automatically updated on each push to this PR.

Copy link
Copy Markdown

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@app/Services/Auth/TwoFactorAuditService.php`:
- Around line 38-43: The Log::debug call in TwoFactorAuditService::log currently
emits raw 'user_id' and 'ip_address'; change it to avoid storing PII by removing
those fields or replacing them with a deterministic, non-reversible token (e.g.,
an HMAC/sha256 or other app-key-based hash) so you can still correlate events
without exposing raw identifiers; update the Log::debug invocation in the
TwoFactorAuditService::log method to keep 'event_type' and 'method' but
redact/hash 'user_id' and 'ip_address' (use the app key or a configured secret
to compute the hash).
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: fe8fe81e-6db5-47e3-a320-1fbaac8283d6

📥 Commits

Reviewing files that changed from the base of the PR and between 4d99419 and ae4e42f.

📒 Files selected for processing (5)
  • app/Services/Auth/ITwoFactorAuditService.php
  • app/Services/Auth/TwoFactorAuditService.php
  • app/Services/Auth/TwoFactorServiceProvider.php
  • phpunit.xml
  • tests/Unit/TwoFactorAuditServiceTest.php

Comment on lines +38 to +43
Log::debug('TwoFactorAuditService::log', [
'user_id' => $user->getId(),
'event_type' => $eventType,
'method' => $method,
'ip_address' => $ipAddress,
]);
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Avoid logging raw user identifiers in debug audit traces.

Line 38 currently logs user_id and ip_address directly. That increases privacy/compliance risk in log storage without being required for core behavior. Prefer removing these fields or redacting/hashing them before logging.

Suggested minimal change
-        Log::debug('TwoFactorAuditService::log', [
-            'user_id' => $user->getId(),
-            'event_type' => $eventType,
-            'method' => $method,
-            'ip_address' => $ipAddress,
-        ]);
+        Log::debug('TwoFactorAuditService::log', [
+            'event_type' => $eventType,
+            'method' => $method,
+        ]);
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
Log::debug('TwoFactorAuditService::log', [
'user_id' => $user->getId(),
'event_type' => $eventType,
'method' => $method,
'ip_address' => $ipAddress,
]);
Log::debug('TwoFactorAuditService::log', [
'event_type' => $eventType,
'method' => $method,
]);
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@app/Services/Auth/TwoFactorAuditService.php` around lines 38 - 43, The
Log::debug call in TwoFactorAuditService::log currently emits raw 'user_id' and
'ip_address'; change it to avoid storing PII by removing those fields or
replacing them with a deterministic, non-reversible token (e.g., an HMAC/sha256
or other app-key-based hash) so you can still correlate events without exposing
raw identifiers; update the Log::debug invocation in the
TwoFactorAuditService::log method to keep 'event_type' and 'method' but
redact/hash 'user_id' and 'ip_address' (use the app key or a configured secret
to compute the hash).

@smarcet smarcet removed the request for review from santipalenque May 27, 2026 11:55
Copy link
Copy Markdown
Contributor

@caseylocker caseylocker left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants